Skip to content

Import check_time_locks() and CoinRecord #19848

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

Import check_time_locks() and CoinRecord #19848

wants to merge 10 commits into from

Conversation

matt-o-how
Copy link
Contributor

@matt-o-how matt-o-how commented Jul 18, 2025 •
edited
Loading

Purpose:

check_time_locks() and CoinRecord have now been ported to Rust so this PR migrates chia-blockchain to use the new port

Must come after a new version of chia_rs implements these changes

Current Behavior:

New Behavior:

Testing Notes:

@matt-o-how matt-o-how added the Changed Required label for PR that categorizes merge commit message as "Changed" for changelog label Jul 18, 2025
@matt-o-how matt-o-how requested a review from wjblanke July 21, 2025 16:06
@wjblanke
Copy link
Contributor

Hey Matt, I'm a little worried that all these rust function calls to access struct members is going to slow everything down (even slower than the old python structure). Is pyo3 efficient at this? Have you done any performance tests?

@matt-o-how matt-o-how marked this pull request as ready for review July 22, 2025 19:38
@matt-o-how matt-o-how requested a review from a team as a code owner July 22, 2025 19:38
@matt-o-how matt-o-how force-pushed the import_check_time_locks branch from df85ac7 to 35e64a6 Compare July 31, 2025 12:30
@matt-o-how
Copy link
Contributor Author

Changed this to no longer require function calls as the Rust implementation now uses #[getter] to mark functions as properties

arvidn
arvidn requested changes Jul 31, 2025
View reviewed changes
@matt-o-how matt-o-how force-pushed the import_check_time_locks branch from 35e64a6 to 0236cf2 Compare August 13, 2025 13:15
@matt-o-how matt-o-how requested a review from altendky as a code owner August 13, 2025 13:15
altendky
altendky requested changes Aug 13, 2025
View reviewed changes
Copy link
Contributor

@altendky altendky left a comment •
edited by matt-o-how
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed this to no longer require function calls as the Rust implementation now uses #[getter] to mark functions as properties

just to be clear, this just avoids the () in code. it doesn't make things faster (at least from a python perspective). my default take on that is that it hides the fact that code is executing and that properies should primarily be used for deprecation of attribute access when you need to add code and are transitioning to using a method exclusively.

but... i think it can be argued that we are doing exactly that here and that the diff is much more comprehensible as such. i might suggest a follow-up though to move away from the getter to a method.

suggested follow-ups, specifically separate for diff clarity around each change:

  • getter to method
  • remove CoinRecord = RustCoinRecord

chia/full_node/mempool_manager.py
self.max_tx_clvm_cost,
self.constants,
self.peak.height,
flags | MEMPOOL_MODE,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's up here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is because this branch is based off Arvid's PR to bump the chia_rs version:
https://github.com/Chia-Network/chia-blockchain/pull/19953/files#diff-ee823ddd491e7037efb6bda95223e6e4aa703239fd08f28c54a6e3a51b75ff42L449

@matt-o-how
Copy link
Contributor Author

#19848 (review)

I agree about removing the old coin_record.py file and the exported re-name. Implemented now.

@github-actions github-actions bot added the merge_conflict Branch has conflicts that prevent merge to main label Aug 13, 2025
@matt-o-how matt-o-how force-pushed the import_check_time_locks branch from d44d4c9 to 3c7e456 Compare August 25, 2025 11:30
@github-actions github-actions bot removed the merge_conflict Branch has conflicts that prevent merge to main label Aug 25, 2025
@github-actions github-actions bot added the merge_conflict Branch has conflicts that prevent merge to main label Sep 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@altendky altendky altendky requested changes

@arvidn arvidn arvidn requested changes

@wjblanke wjblanke Awaiting requested review from wjblanke

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
Changed Required label for PR that categorizes merge commit message as "Changed" for changelog merge_conflict Branch has conflicts that prevent merge to main
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@matt-o-how @wjblanke @altendky @arvidn